[Clang] Allow simpler visibility annotations when targeting win32 and mingw #133699
Conversation
philnik777
force-pushed
the
simplify_dllexport_warnings
branch
from
e417860 to
9a3f8ee
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesMinGW and Win32 disagree on where the Full diff: https://github.com/llvm/llvm-project/pull/133699.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 0999d8065e9f5..6bb8fc05b119a 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4185,6 +4185,14 @@ def DLLExport : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
let Documentation = [DLLExportDocs];
}
+def DLLExportOnDecl : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
+ // This attribute is only used to warn if there was a `__declspec(dllexport)`
+ // on a declaration, but not on the defintion of an explciit instantiation
+ let Spellings = [];
+ let Subjects = SubjectList<[CXXRecord]>;
+ let Documentation = [InternalOnly];
+}
+
def DLLExportStaticLocal : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
// This attribute is used internally only when -fno-dllexport-inlines is
// passed. This attribute is added to inline functions of a class having the
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index b9f08d96151c9..80f27771248bb 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -487,6 +487,9 @@ def Dangling : DiagGroup<"dangling", [DanglingAssignment,
ReturnStackAddress]>;
def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
def DllexportExplicitInstantiationDecl : DiagGroup<"dllexport-explicit-instantiation-decl">;
+def DllexportExplicitInstantiation :
+ DiagGroup<"dllexport-explicit-instantiation",
+ [DllexportExplicitInstantiationDecl]>;
def ExcessInitializers : DiagGroup<"excess-initializers">;
def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
def FlagEnum : DiagGroup<"flag-enum">;
@@ -870,7 +873,8 @@ def NSReturnsMismatch : DiagGroup<"nsreturns-mismatch">;
def IndependentClassAttribute : DiagGroup<"IndependentClass-attribute">;
def UnknownAttributes : DiagGroup<"unknown-attributes">;
-def IgnoredAttributes : DiagGroup<"ignored-attributes">;
+def IgnoredAttributes : DiagGroup<"ignored-attributes",
+ [DllexportExplicitInstantiation]>;
def Attributes : DiagGroup<"attributes", [UnknownAttributes,
IgnoredAttributes]>;
def UnknownSanitizers : DiagGroup<"unknown-sanitizers">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 1e900437d41ce..31324b6b1262f 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3736,9 +3736,19 @@ def warn_attribute_dllimport_static_field_definition : Warning<
def warn_attribute_dllexport_explicit_instantiation_decl : Warning<
"explicit instantiation declaration should not be 'dllexport'">,
InGroup<DllexportExplicitInstantiationDecl>;
-def warn_attribute_dllexport_explicit_instantiation_def : Warning<
+def warn_attr_dllexport_explicit_inst_def : Warning<
"'dllexport' attribute ignored on explicit instantiation definition">,
+ InGroup<DllexportExplicitInstantiation>;
+def warn_attr_dllexport_explicit_inst_def_mismatch : Warning<
+ "'dllexport' attribute ignored on explicit instantiation definition">,
+ InGroup<IgnoredAttributes>;
+def note_prev_decl_missing_dllexport : Note<
+ "'dllexport' attribute is missing on previous declaration">;
+def warn_dllexport_on_decl_ignored : Warning<
+ "explicit instantiation definition is not exported without 'dllexport'">,
InGroup<IgnoredAttributes>;
+def note_dllexport_on_decl : Note<
+ "'dllexport' attribute on the declaration is ignored">;
def warn_attribute_exclude_from_explicit_instantiation_local_class : Warning<
"%0 attribute ignored on local class%select{| member}1">,
InGroup<IgnoredAttributes>;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 676d53a1f4b45..4d69c11678620 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -6606,7 +6606,10 @@ void Sema::checkClassLevelDLLAttribute(CXXRecordDecl *Class) {
if (ClassExported && !ClassAttr->isInherited() &&
TSK == TSK_ExplicitInstantiationDeclaration &&
!Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) {
- Class->dropAttr<DLLExportAttr>();
+ if (auto *DEA = Class->getAttr<DLLExportAttr>()) {
+ Class->addAttr(DLLExportOnDeclAttr::Create(Context, DEA->getLoc()));
+ Class->dropAttr<DLLExportAttr>();
+ }
return;
}
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index be81b6a46b2c0..25d84fde65970 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -9845,13 +9845,29 @@ DeclResult Sema::ActOnExplicitInstantiation(
// mode, if a previous declaration of the instantiation was seen.
for (const ParsedAttr &AL : Attr) {
if (AL.getKind() == ParsedAttr::AT_DLLExport) {
- Diag(AL.getLoc(),
- diag::warn_attribute_dllexport_explicit_instantiation_def);
+ if (PrevDecl->hasAttr<DLLExportAttr>()) {
+ Diag(AL.getLoc(), diag::warn_attr_dllexport_explicit_inst_def);
+ } else {
+ Diag(AL.getLoc(),
+ diag::warn_attr_dllexport_explicit_inst_def_mismatch);
+ Diag(PrevDecl->getLocation(), diag::note_prev_decl_missing_dllexport);
+ }
break;
}
}
}
+ if (TSK == TSK_ExplicitInstantiationDefinition && PrevDecl &&
+ !Context.getTargetInfo().getTriple().isWindowsGNUEnvironment() &&
+ llvm::none_of(Attr, [](const ParsedAttr &AL) {
+ return AL.getKind() == ParsedAttr::AT_DLLExport;
+ })) {
+ if (const auto *DEA = PrevDecl->getAttr<DLLExportOnDeclAttr>()) {
+ Diag(TemplateLoc, diag::warn_dllexport_on_decl_ignored);
+ Diag(DEA->getLoc(), diag::note_dllexport_on_decl);
+ }
+ }
+
if (CheckExplicitInstantiation(*this, ClassTemplate, TemplateNameLoc,
SS.isSet(), TSK))
return true;
diff --git a/clang/test/SemaCXX/dllexport-explicit-instantiation.cpp b/clang/test/SemaCXX/dllexport-explicit-instantiation.cpp
new file mode 100644
index 0000000000000..522e81d37976d
--- /dev/null
+++ b/clang/test/SemaCXX/dllexport-explicit-instantiation.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify=win32,win32-pedantic -DMS %s -Wdllexport-explicit-instantiation
+// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify=win32 -DMS %s -Wno-dllexport-explicit-instantiation
+// RUN: %clang_cc1 -triple x86_64-mingw32 -fsyntax-only -fms-extensions -verify=mingw,mingw-pedantic -DMS %s -Wdllexport-explicit-instantiation
+// RUN: %clang_cc1 -triple x86_64-mingw32 -fsyntax-only -fms-extensions -verify=mingw -DMS %s -Wno-dllexport-explicit-instantiation
+
+template <class>
+class S {};
+
+extern template class __declspec(dllexport) S<short>; // win32-pedantic-warning {{explicit instantiation declaration should not be 'dllexport'}} \
+ win32-pedantic-note {{attribute is here}}
+template class __declspec(dllexport) S<short>; // mingw-pedantic-warning {{'dllexport' attribute ignored on explicit instantiation definition}}
+
+extern template class __declspec(dllexport) S<int>; // win32-pedantic-warning {{explicit instantiation declaration should not be 'dllexport'}} \
+ win32-pedantic-note {{attribute is here}} \
+ win32-note {{'dllexport' attribute on the declaration is ignored}}
+template class S<int>; // win32-warning {{explicit instantiation definition is not exported without 'dllexport'}}
+
+extern template class S<long>; // mingw-note {{'dllexport' attribute is missing on previous declaration}}
+template class __declspec(dllexport) S<long>; // mingw-warning {{'dllexport' attribute ignored on explicit instantiation definition}}
diff --git a/clang/test/SemaCXX/dllexport.cpp b/clang/test/SemaCXX/dllexport.cpp
index 22d92c30954e8..1978e463a3f7a 100644
--- a/clang/test/SemaCXX/dllexport.cpp
+++ b/clang/test/SemaCXX/dllexport.cpp
@@ -462,6 +462,9 @@ template struct ExplicitlyInstantiatedTemplate<int>;
template <typename T> struct ExplicitlyExportInstantiatedTemplate { void func() {} };
template struct __declspec(dllexport) ExplicitlyExportInstantiatedTemplate<int>;
template <typename T> struct ExplicitlyExportDeclaredInstantiatedTemplate { void func() {} };
+#ifdef GNU
+// expected-note@+2 {{attribute is missing}}
+#endif
extern template struct ExplicitlyExportDeclaredInstantiatedTemplate<int>;
#if not defined(MS) && not defined (WI) && not defined(PS)
// expected-warning@+2{{'dllexport' attribute ignored on explicit instantiation definition}}
|
|
|
||
| def DLLExportOnDecl : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> { | ||
| // This attribute is only used to warn if there was a `__declspec(dllexport)` | ||
| // on a declaration, but not on the defintion of an explciit instantiation |
|
I can't really comment much on the implementation here, I would mostly leave that up to people more familiar with those bits in Clang. A little nitpickery wrt the text; it'd be clearer if it'd talk about "MSVC and mingw". Both MSVC mode and mingw mode are equally much "win32" or "windows". It's just that if you specify a triple like Then for the description of the issue, it would be less confusing if it explicitly mentions template instantiations. For simpler things, like a plain function declaration, MSVC mode and mingw mode agree about where the dllexport/import attributes should be placed. Or in practice, mingw mode accepts the attributes in a superset of the locations where msvc mode accepts them. So when reading the subject/description, I first wondered why we need two different things at all, as one place should work for both. But for explicit template instantiations there's inded a bit of conflict in how it needs to be done, and we need the two different approaches. |
| def IgnoredAttributes : DiagGroup<"ignored-attributes", | ||
| [DllexportExplicitInstantiation]>; |
There was a problem hiding this comment.
Can you explain this change? It does not seem correct to me given the attribute has no spelling
There was a problem hiding this comment.
I need some way to track that the attribute has been applied, even though it's being ignored. That's what the new attribute is for. This diagnostic is issued if the attribute has been ignored on the declaration and there is no attribute on the definition. Morally I'm still warning about the __declspec(dllexport), even if it's internally a different attribute.
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Overall I don't mind the patch. The special attribute here is... frustrating/concerning/etc, and I'd rather we just mark the attribute as 'invalid' in some way so that we can diagnose it instead.
| let Documentation = [DLLExportDocs]; | ||
| } | ||
|
|
||
| def DLLExportOnDecl : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> { |
There was a problem hiding this comment.
Hmm... I'm not a huge fan of this TBH. An attribute for the sole purpose of diagnostics is a little novel and not particularly in keeping with our "represent the AST" nature of attributes. Is there really no way we can figure this out later?
There was a problem hiding this comment.
Since we don't represent invalid attribute in the AST currently I don't think there is.
| !Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) { | ||
| Class->dropAttr<DLLExportAttr>(); | ||
| if (auto *DEA = Class->getAttr<DLLExportAttr>()) { | ||
| Class->addAttr(DLLExportOnDeclAttr::Create(Context, DEA->getLoc())); |
There was a problem hiding this comment.
I honestly would like this better if we just kept the DLLExportAttr and added a bool flag to it for ignoredBecauseItsOnaDeclWhereItWouldntWorkForVariousReasons in the extra members section.
There was a problem hiding this comment.
I guess that would be possible. The main problem I see with this is that we'd have to check in a bunch of places whether the attribute is actually valid, and not just whether it exists (AFAICT it's usually assumed that if an attribute is in the AST it is valid).
|
I too had trouble understanding this change based on the description. Could you give a concrete example of how mingw and msvc disagree on where to put the attribute, and explain how this pr changes things? Taking a step back, how will this simplify libc++'s visibility annotations? Don't they still need to be compatible with both mingw and msvc, which we don't control? |
|
They have to be compatible, but we don't need separate macros anymore for exported classes. |
|
ping @erichkeane @cor3ntin @zmodem |
Can you respond to these questions here, they're essential to the motivation of this change IMO. Also, won't the potential simplification in libc++ essentially just amount to this? diff --git a/libcxx/include/__config b/libcxx/include/__config
index 38c47e8d45c8..4102e41ebe23 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -341,13 +341,8 @@ typedef __char32_t char32_t;
# define _LIBCPP_OVERRIDABLE_FUNC_VIS
# define _LIBCPP_EXPORTED_FROM_ABI
# elif defined(_LIBCPP_BUILDING_LIBRARY)
-# if defined(__MINGW32__)
-# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport)
-# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS
-# else
-# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS
-# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS __declspec(dllexport)
-# endif
+# define _LIBCPP_EXTERN_TEMPLATE_TYPE_VIS __declspec(dllexport)
+# define _LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VIS __declspec(dllexport)
# define _LIBCPP_OVERRIDABLE_FUNC_VIS __declspec(dllexport)
# define _LIBCPP_EXPORTED_FROM_ABI __declspec(dllexport)
# elseThe downside is that it would be massively much more confusing to use Clang, as a developer, if it silently accepts |
With this PR Clang doesn't complain in either mode if
No. We can drop
Clang will only accept an incorrect placement silently if there is a correct annotation as well. If it is actually ignored there will be a diagnostic. |
|
Oh, also note that by default there will still be diagnostics about all incorrect placements. You'll have to opt-in to silently ignoring incorrect placements via |
|
Ping |
rnk
left a comment
rnk
left a comment
There was a problem hiding this comment.
It's nearing the end of my workday and I don't have feedback, but I wanted to acknowledge receipt of the PR.
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
No. We can drop
_LIBCPP_EXTERN_TEMPLATE_TYPE_VISand_LIBCPP_CLASS_TEMPLATE_INSTANTIATION_VISby replacing it with_LIBCPP_EXPORTED_FROM_ABI.
But how will that work when building with MinGW or MSVC? Or is that not supported?
I'm a bit conflicted about this. On the one hand, it will make things simpler for developers only building with Clang. On the other hand, it adds new entries to the matrix of how dllexport works across different compilers, which developers may need to understand, and which we need to maintain.
Do we think the benefits are worth the cost here?
If we do move forward with this, I'd ask for some documentation, at least in the form of a release note.
I presume you mean "building with GCC"? (We do build for MinGW targets with Clang all the time.) Building libc++ with MSVC isn't supported - it lacks support for many constructs that libc++ requires. Building libc++ with GCC is tested and supported on Linux. In MinGW environments it should probably work but it's not continuously tested. But iirc @kikairoya did test it quite recently? So if we do require this behaviour from the compiler, we might close the door for using libc++ with GCC on Windows. It may also be possible to just ignore the extra warnings with GCC with some extra flags about ignored attributes though.
Yeah this is probably my main concern as well. |
Is is supported and keeps working exactly the same.
This patch doesn't change any semantics. It doesn't add new entries on any matrix, it simply allows adding attributes in places they are ignored without getting warnings, because what the user expects them to do is already done by an attribute in the correct place. This doesn't affect interoperability with other compilers and you even have to disable warnings about incorrect placements. Note that is does significantly simplify the matrix of required visibility macros for projects primarily targeting Clang though.
I don't see how we do this. It's simply different warnings.
Exactly. We already do this in lots of cases. |
This relies on the assumption that the extra attribute in the unexpected location simply produces a warning, not a hard error without a way to disable it. There are such cases where things that may be a plain warning with Clang is a hard error with GCC - I recently ran into one. But in this case it does indeed seem like the extra attributes are plain warnings with all of Clang, GCC and MSVC. Using the example from above: template <class>
class S {};
extern template class __declspec(dllexport) S<short>; // msvc complains about this __declspec
template class __declspec(dllexport) S<short>; // mingw complains about this __declspec$ clang -target x86_64-windows-msvc -c extra-dllexport.cpp
extra-dllexport.cpp:4:1: warning: explicit instantiation
declaration should not be 'dllexport'
[-Wdllexport-explicit-instantiation-decl]
4 | extern template class __declspec(dllexport) S<short>; // msvc complains ...
| ^
extra-dllexport.cpp:4:34: note: attribute is here
4 | extern template class __declspec(dllexport) S<short>; // msvc complains ...
| ^
1 warning generated.
$ clang -target x86_64-windows-gnu -c extra-dllexport.cpp
extra-dllexport.cpp:5:27: warning: 'dllexport' attribute
ignored on explicit instantiation definition [-Wignored-attributes]
5 | template class __declspec(dllexport) S<short>; // mingw complain...
| ^
1 warning generated.
$ x86_64-w64-mingw32-g++ -c extra-dllexport.cpp
extra-dllexport.cpp:5:38: warning: type attributes ignored after type is already defined [-Wattributes]
5 | template class __declspec(dllexport) S<short>; // mingw complains about this __declspec
| ^~~~~~~~
$ cl -c extra-dllexport.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.44.35207.1 for x64
Copyright (C) Microsoft Corporation. All rights reserved.
extra-dllexport.cpp
extra-dllexport.cpp(4): warning C4910: 'S<short>': '__declspec(dllexport)' and 'extern' are incompatible on an explicit instantiation |
I believe #141040 allows that to build/link with GCC. |
AFAIK, building libc++ with MinGW-GCC requires a tweak ( a workaround for lacking |
|
ping |
|
ping @cor3ntin @erichkeane |
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Seems you missed two things, and I'd like you to improve the hygiene of the test (or at least, not make it worse).
|
|
||
| def DLLExportOnDecl : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> { | ||
| // This attribute is only used to warn if there was a `__declspec(dllexport)` | ||
| // on a declaration, but not on the defintion of an explciit instantiation |
| @@ -462,6 +462,9 @@ template struct ExplicitlyInstantiatedTemplate<int>; | |||
| template <typename T> struct ExplicitlyExportInstantiatedTemplate { void func() {} }; | |||
| template struct __declspec(dllexport) ExplicitlyExportInstantiatedTemplate<int>; | |||
| template <typename T> struct ExplicitlyExportDeclaredInstantiatedTemplate { void func() {} }; | |||
| #ifdef GNU | |||
There was a problem hiding this comment.
Urgh, I hate macros for this purpose, we have the verify= flag for these things, and this file is awful about this...
Could you instead of 'making it worse' (even though only slightly), change it to -verify=expected,gnu and change these to //gnu-note@... instead?
I realize the rest of the file is still badly done, but it is at least... a step in the right direction.
9 participants
MinGW and Win32 disagree on where the
__declspec(dllexport)should be placed on extern template instantiations. However, there doesn't fundamentally seem to be a problem with putting the annotation in both places. This patch adds a new diagnostic group and-Wattribute-ignoredwarnings about where the attribute is placed if the attribute is different on the declaration and definition. There is another new warning group-Wdllexport-explicit-instantiationthat also diagnoses places where the attribute is technically ignored, even though the correct place is also annotated. This makes it possible to significantly simplify libc++'s visibility annotations (see #133704).